Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: checkpoint error with Azure Synapse #1848

Merged
merged 1 commit into from
Nov 16, 2023
Merged

fix: checkpoint error with Azure Synapse #1848

merged 1 commit into from
Nov 16, 2023

Conversation

PierreDubrulle
Copy link
Contributor

@PierreDubrulle PierreDubrulle commented Nov 13, 2023 •
edited
Loading

Description

Modify Checkpoint stuct not to serialize optional fields if null.

Related Issue(s)

@github-actions github-actions bot added binding/rust Issues for the Rust crate crate/core labels Nov 13, 2023
@github-actions GitHub Actions
Copy link

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@PierreDubrulle PierreDubrulle changed the title Remove null checkpoints fix: Checkpoint error with Azure Synapse Nov 13, 2023
@PierreDubrulle PierreDubrulle changed the title fix: Checkpoint error with Azure Synapse fix: checkpoint error with Azure Synapse Nov 13, 2023
rtyler
rtyler previously approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@rtyler rtyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double-checked the protocol for this one, and I think the skipping on serialization is fine.

Kind of annoying that Azure Synapse is being doof, this has also come up in discussions

I trust that you've tested this change with Azure Synapse?

@rtyler
Copy link
Member

rtyler commented Nov 16, 2023

I have fixed some minor issues and rebased on top of main

@PierreDubrulle
Copy link
Contributor Author

@rtyler
Yes, it's really the fields set to null that prevent the table from being read in Azure Synapse.

Skip_serializing_if is surely the best option to mitigate this problem, as it has the least impact on the rest of the code.

@rtyler rtyler added this to the Rust v0.17 milestone Nov 16, 2023
@rtyler rtyler merged commit 691ed8b into delta-io:main Nov 16, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rtyler rtyler rtyler approved these changes

@wjones127 wjones127 Awaiting requested review from wjones127 wjones127 is a code owner

@roeap roeap Awaiting requested review from roeap roeap is a code owner

Assignees
No one assigned
Labels
binding/rust Issues for the Rust crate crate/core
Projects
None yet
Milestone
Rust v0.17
Development

Successfully merging this pull request may close these issues.

Checkpoint error with Azure Synapse
2 participants
@PierreDubrulle @rtyler